Use aws-lc-rs instead of ring for TLS by kcon-stackav · Pull Request #4734 · astral-sh/uv

kcon-stackav · 2024-07-02T18:39:32Z

Summary

This PR switches the TLS backend used by reqwest from ring to aws-lc to support more SSL certificate signature algorithms (especially P521 algorithms which aren't yet supported by ring: briansmith/ring#1631).

Fixes #4534

Test Plan

I used uv pip install to try installing a package from a private PyPI server whose SSL certificate was signed using the ECDSA SHA-512 certificate signature algorithm and, using the Rust debugger, observed that uv did not fail to install the package due to not supporting the ECDSA SHA-512 certificate signature algorithm.

zanieb · 2024-07-02T20:20:42Z

Thanks for the pull request!

zanieb · 2024-07-02T20:36:11Z

This Windows failure also looks somewhat problematic (see aws/aws-lc#1477 and example fix)

kcon-stackav · 2024-07-02T22:32:54Z

+      - name: "Install nasm"
+        uses: ilammy/setup-nasm@v1


@zanieb I'm not very familiar with maturin, do you think installing NASM is needed for the build-binaries Windows job too?

uv/.github/workflows/build-binaries.yml

Lines 153 to 162 in 66a4b8e

windows:

if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-build') }}

runs-on: windows-latest

strategy:

matrix:

platform:

- target: x86_64-pc-windows-msvc

arch: x64

- target: i686-pc-windows-msvc

arch: x86

cc @konstin

maturin calls cargo build, so they same rules should apply whether it's maturin or not.

zanieb · 2024-07-02T22:42:52Z

          Copy-Item -Path "${{ github.workspace }}" -Destination "${{ env.DEV_DRIVE }}/uv" -Recurse

+      - name: "Install nasm"
+        uses: ilammy/setup-nasm@v1


Note to self we should audit this action and consider just implementing it ourself if the install is straightforward

ilammy also known for https://github.com/ilammy/msvc-dev-cmd

zanieb · 2024-07-02T23:25:44Z

+                if aws_lc_rs::default_provider().install_default().is_err() {
+                    warn_user_once!("Failed to install aws_lc_rs as the default TLS provider.");
+                }


It looks like the error attached to the result is just Self and has no information, right?

Yes I understand the error holds the CryptoProvider that was previously installed as the default in that case.

I guess we could capture it to say what we're using instead but it doesn't seem critical.

Ah yeah I thought about doing that but I wasn't sure the CryptoProvider had a nice human-readable name field we could use: https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html

zanieb · 2024-07-10T14:59:45Z

Sorry this is lingering, I'm just not sure of the trade-offs here since it changes our release pipeline and hasn't been requested by many people.

kcon-stackav · 2024-07-15T17:47:13Z

Sorry this is lingering, I'm just not sure of the trade-offs here since it changes our release pipeline and hasn't been requested by many people.

No worries, and feel free to close this PR if not enough people are wanting it since I learned that switching from ring to aws-lc-rs doesn't solve my particular problem after all (#4534 (comment)), but I believe #1339 should once it becomes available.

zanieb · 2024-07-19T17:03:21Z

I think I'll close for now since this changes our build dependencies and there isn't a compelling reason to switch over at this time. Happy to reconsider in the future.

rami3l · 2024-08-01T10:55:35Z

@zanieb As a part of Rustup's plan for the v1.28.0 release cycle, I've migrated Rustup to aws-lc-rs in rust-lang/rustup#3898 for the very same reason (rust-lang/rustup#3820), and I've been contacting aws-lc's side, providing useful information to help address various issues regarding the cross compilation of this library. I'd love to share more experience with you when we're able to actually ship the build, if you're interested.

zanieb · 2024-08-01T12:24:44Z

Thanks @rami3l, I appreciate the heads up and am definitely interested.

cyberksh · 2026-01-02T14:54:30Z

Hello are we planning to add support for this?
If this could be an optional feature for Linux only or we could use the prebuilt NASM modules that are provided by aws-lc-rs

rami3l · 2026-01-02T15:05:26Z

Thanks @rami3l, I appreciate the heads up and am definitely interested.

@zanieb Sorry for the long wait, but as you might have noticed, we've been using aws-lc-rs in production for about a year now. The team has been quite responsive in terms of aligning our supported targets so it might be worth it to make the move.

kcon-stackav changed the title ~~Explore using aws-lc-rs instead of ring for TLS~~ Use aws-lc-rs instead of ring for TLS Jul 2, 2024

zanieb self-assigned this Jul 2, 2024

zanieb added the network Network connectivity e.g. proxies, DNS, and SSL label Jul 2, 2024

zanieb reviewed Jul 2, 2024

View reviewed changes

Comment thread crates/uv-client/src/base_client.rs Outdated

kcon-stackav added 2 commits July 2, 2024 22:07

Explore using aws-lc-rs instead of ring for TLS

075423e

Fix fmt and avoid setting OnceCell more than once

f25c467

kcon-stackav force-pushed the kcon-stackav/use-aws-lc-instead-of-ring branch from 7f091b3 to 34852fe Compare July 2, 2024 22:07

kcon-stackav added 3 commits July 2, 2024 22:15

Add NASM to Windows builds

58e7c42

Follow Clippy's advice to use let instead of match

705ad51

Increase size of Windows dev drive in test build

7fdb31c

kcon-stackav force-pushed the kcon-stackav/use-aws-lc-instead-of-ring branch from 34852fe to 7fdb31c Compare July 2, 2024 22:20

Simplify code further for Clippy

616673b

kcon-stackav commented Jul 2, 2024

View reviewed changes

kcon-stackav marked this pull request as ready for review July 2, 2024 22:38

zanieb reviewed Jul 2, 2024

View reviewed changes

Comment thread crates/uv-client/src/base_client.rs Outdated

kcon-stackav mentioned this pull request Jul 2, 2024

invalid peer certificate: BadSignature when installing package from private index using ECDSA SHA-512 SSL cert #4534

Closed

kcon-stackav added 2 commits July 2, 2024 22:49

Make Windows dev drive even larger for test job

12850cc

Warn instead of expecting

cf73ab5

zanieb reviewed Jul 2, 2024

View reviewed changes

zanieb added the needs-decision Undecided if this should be done label Jul 15, 2024

zanieb closed this Jul 19, 2024

	windows:
	if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-build') }}
	runs-on: windows-latest
	strategy:
	matrix:
	platform:
	- target: x86_64-pc-windows-msvc
	arch: x64
	- target: i686-pc-windows-msvc
	arch: x86

Conversation

kcon-stackav commented Jul 2, 2024

Summary

Test Plan

Uh oh!

Uh oh!

zanieb commented Jul 2, 2024

Uh oh!

zanieb commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanieb commented Jul 10, 2024

Uh oh!

kcon-stackav commented Jul 15, 2024

Uh oh!

zanieb commented Jul 19, 2024

Uh oh!

rami3l commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanieb commented Aug 1, 2024

Uh oh!

cyberksh commented Jan 2, 2026

Uh oh!

rami3l commented Jan 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zanieb commented Jul 2, 2024 •

edited

Loading

rami3l commented Aug 1, 2024 •

edited

Loading